-
Notifications
You must be signed in to change notification settings - Fork 227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
proto: Align TryFrom
impls for Duration
with prost-types
#1456
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this probably makes more sense than supporting negative durations, at least unless there's an actual use for them in the Cosmos ecosystem somewhere
@romac with this merged I can confirm I can upgrade |
Glad to hear! Will do a release tomorrow then |
Will do! Apologies for the oversight and thanks for all the PRs 🙏 |
Coming from the above ibc-rs issue. Because of this PR, we can't expect tendermint-rs/proto/src/lib.rs Lines 105 to 106 in 2f94e7f
should be refactored to something like pub trait Protobuf<T: Message + TryFrom<Self> + Default>
where Should I open an issue for this? Or I missed anything from the latest releases? |
Ah that's an oversight from my part, sorry about that. Not sure what to do about that though? Do you think can make that conversion infallible for Duration? |
I just explored the idea of I am going to open an issue for this and continue the discussion there. |
Like Rano said, this broke a general ecosystem pattern of infallible domain type to wire conversion. To route around this, we are newtyping the core duration and making the conversion infallible: penumbra-zone/ibc-types#94. This is kind of ugly but works. I don't think we should let this change contaminate the rest of the ecosystem types by changing the Without commenting on this specific change, it would be awesome to have more heads up/consensus when we change domain type modeling or APIs. This is because the smallest change has dangerous ripple effects throughout. In most cases, downstream crates can navigate those change nimbly, but they are more perilous for deployed systems that need to plan around consensus/state compatibility. That's the case for Penumbra, Astria, Namada, and I'm sure many others as well. |
Agreed, we should probably revert the change and make the conversion infallible again. Maybe we can provide a newtype type that is fallible in case someone needs to make sure that the conversion does not saturate? |
No description provided.